-
-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow direct reuse of dry-validation contracts in actions #454
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
timriley
force-pushed
the
validation-contract-reuse
branch
from
September 2, 2024 14:13
85599da
to
bd39600
Compare
This way we don’t need to have a base validator contract to permit it.
Now that our CSRF protection module pulls the _csrf_token directly from the raw params, we don’t need a vase validator, and can use dry-validation contracts directly
timriley
force-pushed
the
validation-contract-reuse
branch
from
September 2, 2024 14:23
bd39600
to
919ff62
Compare
timriley
force-pushed
the
validation-contract-reuse
branch
from
September 3, 2024 11:18
919ff62
to
9f6ab88
Compare
I think it’s better we use consistent terminology throughout the codebase.
@alassek Going to ping you for a review here, since I headed in this direction after your recent prompting. How does feel to you? Does it give you what you'd hoped for? |
Merging this one now. We can always make further adjustments later. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow on from #453, and allow dry-validation contract classes to be provided directly to actions, in two ways.
First, via the contract class method, instead of expecting
Hanami::Action::Params
subclasses:Second, via a
contract
dependency of the action:This is an altogether more flexible arrangement, since it will allow contracts to be used across both actions and other non-action classes within Hanami apps.
With this PR, we "soft deprecate" the idea of using
Hanami::Action::Params
subclasses as the way to provide reusable validation contracts to actions. We do this by updatingHanami::Actions.contract
such that it does not support being givenHanami::Action::Params
subclasses. Given that.contract
is the new and full-powered incarnation of action validation, this hopefully will nudge people in the right direction.We also have #455 for us to consider a proper deprecation of all direct
Hanami::Action::Params
usage, including subclasses being given toAction.params
.Implementation-wise, here's how we've done this:
contract_class
setting toHanami::Action
Action.params
andAction.contract
set thiscontract_class
configAction#initialize
, initialize thecontract_class
and assign it to@contract
— because we initialize the contract at this point, rather than at the time of assignment, it means we support contracts with their own auto-injected dependencies!@contract
into the created instance ofHanami::Action::Params
Hanami::Action::Params
to validate its params via this contract, or when none is given (i.e. when the action itself has no user-supplied contract), via an internal default contract that permits all params and deep symbolizes the keysThat's it! Along the way, we've also been able to simplify things:
Hanami::Action::Params
subclass (asparams_class
) for every action with a contract defined (instead we store the contract_class and pass it into a standard params object, as described above)Hanami::Action::Params
andHanami::Action::BaseParams
, while still ensuring thatParams
on its own can still work when the hanami-validations gem is not bundled_csrf_token
param is permitted. This would have been an issue with reusable user-defined contracts, since users will want to inherit fromDry::Validation::Contract
directly, not a base contract from hanami-controller. To account for this, update theCSRFProtection
module so that it looks for the_csrf_token
in the raw, non-validated params instead.Not only does this change simplify internals, it also offers a net reduction in lines of code! 😄